add docs #263

ghost · 2018-10-16T07:40:58Z

Edit Nov 11 5pm

Thank you all for the feedback. I'm working on Keanu's fork for now: https://github.com/keanulee/lit-element/

The docs site is staged here: https://polymer-lit-element-staging.appspot.com/

docs: Main docs folder. gh-pages will build the docs site from Jekyll source in here. Only the source files (listed below) need review :)

These folders/files need review:

docs/_data: Jekyll yaml files
docs/_includes/*.html: Jekyll templates
docs/_layouts: Jekyll templates
docs/css: Styles source
docs/docs: Markdown source for the API Documentation section of the site
~~docs/projects~~ docs/_includes/projects: Source files for embedded StackBlitz code samples. ~~(This folder also gets copied* into docs/_includes/.)~~
docs/src: Source for the code to do the embedded StackBlitz samples.
docs/tools: Markdown source for the Tools section of the site.
docs/try: Markdown source for the "Try LitElement" tutorial section of the site

What is the other stuff & why is it here??

Other stuff is build output required for the docs site to run; doesn't need to be reviewed. E.g.

docs/_site: Generated Jekyll output
docs/build: Build output - needed for StackBlitz to run in gh-pages
docs/_includes/projects: Duplicate of content in docs/projects because Jekyll can't find them if they're not here, and my src/* code can't find them if they are here. I need a better way of serving these files :)

ghost · 2018-10-16T07:41:19Z

Demo: https://katejeffreys-projects.github.io/lit-element/

kenchris · 2018-10-16T08:07:17Z

docs/_introduction.md

+  super();
+  this.myProp='there';
+  this.setTimeout(() => {
+    this.myProp='world';


spaces around = ?

eskan · 2018-10-16T08:07:27Z

docs/_site/index.html

+<p>Learn more:</p>
+
+<ul>
+  <li><a href="/try/">Try lit-element</a> without intalling anything.</li>


typo : intalling -> installing

kenchris · 2018-10-16T08:07:37Z

docs/_introduction.md

+```js
+render({}){
+  return html`
+    <button on-click="${(e) => this._clickHandler(e)}"></button>


this is @click now

Some information on what this is would be useful, as there is special handing in lit-html for that

kenchris · 2018-10-16T08:08:19Z

docs/_introduction.md

+    <button on-click="${(e) => this._clickHandler(e)}"></button>
+  `;
+}
+_clickHander(e){


space before {

kenchris · 2018-10-16T08:09:03Z

docs/_site/build/gh-pages/src/index.html

+
+  </article>
+
+


an excessive amount of newlines here

kenchris · 2018-10-16T08:09:29Z

docs/_site/build/gh-pages/src/project-displayer.bak

+    this.project={};
+  }
+  render(){
+    var filenames=[];


don't use var today :-)

Also I would add spaces around assignment as well as between () and { - that seems like common style

kenchris · 2018-10-16T08:10:18Z

docs/_site/build/gh-pages/src/project-displayer.bak

+  }
+  render(){
+    var filenames=[];
+    if(this.project.files) var filenames=Object.keys(this.project.files);


as well as after if which isn't a function

kenchris · 2018-10-16T08:10:35Z

docs/_site/build/gh-pages/src/project-displayer.bak

+    this.shadowRoot.getElementById('toggle').disabled = false;
+  }
+  async embedProject(project){
+    var slot = this.shadowRoot.getElementById("slot");


kenchris · 2018-10-16T08:11:14Z

docs/_site/build/gh-pages/src/project-loader.js

+    this.loadProject();
+  }
+
+  loadProject() {


async loadProject and then use of async/await would make the below easier to read

kenchris · 2018-10-16T08:11:31Z

docs/_site/build/gh-pages/src/project-loader.js

+  }
+
+  loadProject() {
+    if (this.folder == 'undefined') {


why the 's ?

kenchris · 2018-10-16T08:12:17Z

docs/_site/docs/create/design.html

+  <li>
+    <a href="/try/">Try lit-element</a>
+
+


what is up with all these newlines?

Jekyll/Liquid does it. Will fix. :)

43081j · 2018-10-16T09:26:51Z

docs/_site/build/gh-pages/src/project-displayer.bak

@@ -0,0 +1,132 @@
+import {LitElement, html} from '@polymer/lit-element';


.bak, is this file meant to be here?

elarb · 2018-10-16T09:30:50Z

docs/_site/build/gh-pages/src/project-loader.js

+  }
+
+}
+;


This semicolon is probably unnecessary

elarb · 2018-10-16T09:31:35Z

docs/_site/build/gh-pages/src/stack-blitz.js

+
+}
+
+;


This semicolon is probably unnecessary as well

elarb · 2018-10-16T09:33:26Z

docs/_site/build/gh-pages/src/stack-blitz.js

+
+  embedProject(project, options) {
+    var embedIn = this.shadowRoot.getElementById('stackblitz');
+    const vm = sdk.embedProject(embedIn, project, options);


vm doesn't seem to be used

kenchris · 2018-10-16T09:38:38Z

docs/_introduction.md

+Create custom elements that update on property changes. lit-element creates DOM once, then re-renders only the stuff that changed - making DOM updates lightning-fast.
+
+```js
+static get properties(){


space between () and {

43081j · 2018-10-16T09:38:44Z

@katejeffreys you may want to mention what part of this people should be reviewing.

unless im mistaken, a lot of this is build output so shouldn't be code reviewed, rather the sources should. seems those are in docs/*.

kenchris · 2018-10-16T09:39:11Z

docs/_introduction.md

+
+## React to property changes
+
+Create custom elements that update on property changes. lit-element creates DOM once, then re-renders only the stuff that changed - making DOM updates lightning-fast.


elarb · 2018-10-16T09:39:12Z

docs/_site/index.html

+<p><strong>Create fast, lightweight, reusable web components that work in any web page:</strong></p>
+
+<div class="language-js highlighter-rouge"><div class="highlight"><pre class="highlight"><code><span class="kd">class</span> <span class="nx">MyElement</span> <span class="kd">extends</span> <span class="nx">LitElement</span><span class="p">{</span>
+  <span class="c1">// implemnt MyElement</span>


typo: implemnt -> implement

kenchris · 2018-10-16T09:40:00Z

docs/_introduction.md

+
+<a id="expressions"></a>
+
+## Use simple JavaScript expressions for loops and conditionals


why simple? You can use advanced as well :-)

Use well-known JavaScript expressions for loops and conditionals

kenchris · 2018-10-16T09:40:25Z

docs/_introduction.md

+
+## Use simple JavaScript expressions for loops and conditionals
+
+Handling conditionals and loops in your lit-element templates is easy. No special annotations, just plain JavaScript expressions:


s/plain/regular?

Think I'll just remove "plain"

kenchris · 2018-10-16T09:40:57Z

docs/_introduction.md

+    <ul>
+      ${myArray.map(i => html`<li>${i}</li>`)}
+    </ul>
+    ${myBool?


Add space before ? as well as before : - that makes it must easier to read and spot what you are doing

kenchris · 2018-10-16T09:41:19Z

docs/_introduction.md

+Use JavaScript expressions to add event listeners in plain HTML:
+
+```js
+render({}){


I don't think render takes arguments anymore

kenchris · 2018-10-16T09:42:19Z

docs/_site/build/gh-pages/src/project-displayer.bak

+    return html`
+      <style>
+        :host, #staticsample { 
+          min-height:50vh; 


space after :

kenchris · 2018-10-16T09:46:37Z

docs/_site/docs/templates/design.html

+<div class="language-text highlighter-rouge"><div class="highlight"><pre class="highlight"><code>import { LitElement, html } from '@polymer/lit-element';
+
+/**
+ * Anti-pattern. Avoid manipulating DOM.


Not sure what you mean.

Lit-html manipulates the DOM, so the anti-pattern is that people manually manipulate it

43081j · 2018-10-17T21:44:45Z

I opened #270 related to the formatting. If we figure that out we can run it again on the docs JS too.

Until then though it looks like it'll mangle a lot of your html templates and what not 😢

e111077 · 2018-10-17T22:01:10Z

docs/_includes/prevnext.html

+<div class="prevnext">
+  <div class="next">
+    {%- if include.nexturl -%}
+    <a href="{{include.nexturl}}">Next: {{include.nexttitle}} &gt;&gt;</a>


Something wonky is going on here with escaping. see image:

e111077

oops disregard my review. I meant to leave that as a comment

keanulee · 2018-10-18T19:58:19Z

docs/docs/import.md

+
+```html
+<head>
+  <script type="module" src="node_modules/package-name/existing-element.js"></script>


I would suggest importing by name with inline script instead and let the tools will handle the resolution:

<script type=module> import 'package-name/existing-element.js'; </script>

This is what we did for PolymerElement demos (e.g. https://github.com/PolymerElements/iron-list/blob/master/demo/index.html#L20).

keanulee · 2018-10-19T23:07:16Z

docs/projects/ removed; docs/_includes/projects/ is the single source for examples now.

e111077 · 2018-10-24T19:45:48Z

docs/src/stack-blitz.js

+  async loadProject() {
+    const folder = this.folder;
+    if (folder) {
+      const response = await fetch(`${folder}/manifest.json`);


Does this have problems with multiple clicks? Can you disable the button and add visual feedback that the click was registered?

andria-dev · 2018-10-29T08:56:14Z

docs/_includes/projects/docs/expressions/custom-element.js

+      <ul>
+        ${this.myArray.map(i => html`<li>${i}</li>`)}
+      </ul>
+      ${this.myBool?


I would suggest moving the ? and : to the begging of each result for better readability like so:

${this.myBool ? html`<p>Render some HTML if myBool is true</p>` : html`<p>Render some other HTML if myBool is false</p>` }

I like this, will edit samples accordingly if I get time.

andria-dev · 2018-10-29T09:03:28Z

docs/_includes/projects/docs/lifecycle/first/my-element.js

+    `;
+  }
+  firstUpdated(changedProperties){
+    console.log(changedProperties);


Why are you doing console.log(changedProperties) here? It seems unnecessary.

Sometimes I like to do console output to illustrate things to the reader. It's just a sample and they can ignore it if they want.

andria-dev · 2018-10-29T09:04:11Z

docs/_includes/projects/docs/lifecycle/first/my-element.js

+    `;
+  }
+  firstUpdated(changedProperties){
+    console.log(changedProperties);


I'm not 100% on this but I believe you're supposed to call super.firstUpdated(changedProperties) when you use firstUpdated

I didn't see anything to suggest this in the code or readme.

You're right, my bad

andria-dev · 2018-10-29T09:09:08Z

docs/_includes/projects/docs/templates1/my-element.js

+  }
+  changeMe(prop){
+    var rand = Math.floor(Math.random()*100);
+    prop=='prop1'?this.prop1=rand:this.prop2=rand;


Should either be an if statement or can be simplified into:

Suggested change

prop=='prop1'?this.prop1=rand:this.prop2=rand;

this[prop] = rand;

andria-dev · 2018-10-29T09:17:11Z

docs/_includes/projects/try/style/after/my-element.js

+      <!-- Style this text --> 
+      <p class="${this.myBool?'red':'blue'}">style me</p>
+      
+      <button @click="${(event) => this.clickHandler(event)}">Click</button>


It seems redundant to wrap a method in an arrow function. Instead you can just bind this.clickHandler:

Suggested change

<button @click="${(event) => this.clickHandler(event)}">Click</button>

<button @click="${this.clickHandler.bind(this)}">Click</button>

With the recent "event context" addition, this should just be ${this.clickHandler} to avoid having to call .bind() each render. See #244

andria-dev · 2018-10-29T09:18:39Z

docs/_includes/projects/try/style/after/my-element.js

+
+      <p>${this.message}</p>
+
+      <ul>${this.myArray.map(i => html`<li>${i}</li>`)}</ul>


The use of i here could be potentially confusing as i usually stands for index. Maybe it or text would be better here.

good point, will edit

ghost · 2018-11-12T10:14:44Z

Closing this PR because it will be better to raise from Keanu's fork.

Kate Jeffreys added 3 commits October 16, 2018 00:13

add docs

e205f51

add includes

296ed71

fix config

f5c4219

ghost requested a review from sorvell as a code owner October 16, 2018 07:40

kenchris reviewed Oct 16, 2018

View reviewed changes

docs/_introduction.md Outdated

super();

this.myProp='there';

this.setTimeout(() => {

this.myProp='world';

Copy link

Contributor

kenchris Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces around = ?

eskan reviewed Oct 16, 2018

View reviewed changes

kenchris reviewed Oct 16, 2018

View reviewed changes

docs/_site/build/gh-pages/src/index.html

</article>

Copy link

Contributor

kenchris Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an excessive amount of newlines here

kenchris reviewed Oct 16, 2018

View reviewed changes

docs/_site/build/gh-pages/src/project-loader.js Outdated

}

loadProject() {

if (this.folder == 'undefined') {

Copy link

Contributor

kenchris Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the 's ?

kenchris reviewed Oct 16, 2018

View reviewed changes

43081j reviewed Oct 16, 2018

View reviewed changes

elarb reviewed Oct 16, 2018

View reviewed changes

docs/_site/build/gh-pages/src/project-loader.js Outdated

}

}

;

Copy link

elarb Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This semicolon is probably unnecessary

elarb reviewed Oct 16, 2018

View reviewed changes

docs/_site/build/gh-pages/src/stack-blitz.js Outdated

}

;

Copy link

elarb Oct 16, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This semicolon is probably unnecessary as well

elarb reviewed Oct 16, 2018

View reviewed changes

kenchris reviewed Oct 16, 2018

View reviewed changes

elarb reviewed Oct 16, 2018

View reviewed changes

kenchris reviewed Oct 16, 2018

View reviewed changes

roll back stuff and remove _site from repo

434f4fd

43081j mentioned this pull request Oct 17, 2018

Use async/await in project-loader in docs #269

Closed

e111077 requested changes Oct 17, 2018

View reviewed changes

e111077 approved these changes Oct 17, 2018

View reviewed changes

feedback and content

96ee542

keanulee reviewed Oct 18, 2018

View reviewed changes

keanulee added 3 commits October 19, 2018 12:33

Bundle scripts; consolidate annotations example

584dfbf

Output _includes as a collection

83f7c84

Remove duplicate projects/

af281ed

ghost mentioned this pull request Oct 19, 2018

[docs] change code samples to import by name with inline script #273

Closed

keanulee added 2 commits October 22, 2018 15:58

Refactor StackBlitz embed

5678cb3

Update docs/_includes/prevnext.html

58d6079

googlebot added the cla: yes label Oct 24, 2018

e111077 reviewed Oct 24, 2018

View reviewed changes

keanulee added 3 commits October 24, 2018 15:34

stack-blitz loading state

4621ed2

Port layout/styles from other docs

e26e45b

Remove custom font

202bb16

andria-dev reviewed Oct 29, 2018

View reviewed changes

lit deleted a comment from 43081j Nov 12, 2018

lit deleted a comment from keanulee Nov 12, 2018

ghost closed this Nov 12, 2018

ghost deleted the docs branch December 4, 2018 03:28

This pull request was closed.

		@@ -0,0 +1,132 @@
		import {LitElement, html} from '@polymer/lit-element';

+                }
+              }
+              ;


		## React to property changes

		Create custom elements that update on property changes. lit-element creates DOM once, then re-renders only the stuff that changed - making DOM updates lightning-fast.


		<a id="expressions"></a>

		## Use simple JavaScript expressions for loops and conditionals


		## Use simple JavaScript expressions for loops and conditionals

		Handling conditionals and loops in your lit-element templates is easy. No special annotations, just plain JavaScript expressions:

	prop=='prop1'?this.prop1=rand:this.prop2=rand;
	this[prop] = rand;

	<button @click="${(event) => this.clickHandler(event)}">Click</button>
	<button @click="${this.clickHandler.bind(this)}">Click</button>


		<p>${this.message}</p>

		<ul>${this.myArray.map(i => html`<li>${i}</li>`)}</ul>

add docs #263

add docs #263

Uh oh!

Conversation

ghost commented Oct 16, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

43081j Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elarb Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

43081j commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

43081j commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

e111077 left a comment

ghost commented Oct 16, 2018 •

edited by ghost

Loading

43081j Oct 16, 2018 •

edited

Loading

elarb Oct 16, 2018 •

edited

Loading